-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use NPM, node ES5, and browserify. #268
Conversation
I put all of the large drone work in the first commit, later commits are where all the good review needed stuff is at... |
This still builds concatenated js with browserify? |
Yeah it's concatenated. One problem with this approach is that it's currently backwards incompatible with require.js based code (i.e. ipywidgets). With a little more thought, and maybe leaving require.js as a dependency, I should be able to keep backwards compatibility. |
This is crazy and I'm excited to try it out. |
missing = [ m[len(prefix):] for m in missing ] | ||
raise ValueError("The following required files are missing: %s" % missing) | ||
|
||
log.warn("rebuilding js and css failed.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a quote at the end here (though there are other issues too).
How long is the build step ? Is there a way to not have a build step and bundle and separate file to iterate over the notebook quickly ? The 30 s build step is annoying. Potentially a dev mode where files are actually es6 ? |
@Carreau 9.080s, I plan on implementing a |
It will get better as we break things apart better. This is purely migrating what we have right now. We also don't have to do the full bundle if we don't want to (can split apart and only rebuild small steps). For now Jon is tackling the conversion so it works then we must get into a nice live reload quick differential rebuild stage. |
That seem like a plan ! Also side not, you might want not to remove indentation, otherwise rebase On Tue, Aug 11, 2015 at 6:17 PM, Kyle Kelley notifications@github.com
|
Yay, build is working successfully. I'm getting roughly ~8s per full build |
Awesome, thanks for investigating better ways to work with Javascript. |
Thanks for the suggestion, I rebase -i editted the commit with the de-indent, re-indenting everything. This should make the diff much more readable! We can open up a dedicated de-indent PR later. |
"jquery": "components/jquery#~2.0", | ||
"jquery-ui": "components/jqueryui#~1.10", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON doesn't support comments, so I'm leaving one here in GitHub. The following packages can be removed from bower and the setupbase check now, but I left them here for backwards compatibility... In case any users are loading them in via requirejs.
Even though there are still some JS errors on the notebook page and the tests aren't passing yet, the apps are working now. |
To maintain backwards compatibility with JS extensions that use requirejs, we could use a modified version of this: https://github.com/guybedford/cjs Alternatively, we could use r.js to go from commonjs->amd quickly - much quicker than the bundling operation we did before. |
@minrk @Carreau @rgbkrk should I continue to pursue this? I'm trying to gauge interest. I've figured out a way to make it backwards compatible with existing notebook extensions using require.js (i.e. ipywidgets), I'd need a custom require.js extension. I'm just wondering, if got this to work, if it would be something that we'd be interested in merging soon (< month). This would allow us to proceed with the idea of a gradual refactor, instead of a complete rewrite. We could start by trying the output area Kyle and I have pushed. Otherwise we can mothball this, and revisit later if it's just too soon... |
Very cool! Is everyone agreed that we want to go this way rather than require.js? What about code that needs to be loaded asynchronously and not on first page load? If this can help us start to refactor in place and move more quickly, I think it would be a great thing. |
@ellisonbg , yup, that works now. I had to do that for contents. If you want code to load dynamically, you call |
Great! What type of review do you think this needs? It is big and risks bit On Thu, Aug 13, 2015 at 2:33 PM, Jonathan Frederic <notifications@github.com
Brian E. Granger |
Well the biggest thing I need is agreement on transpiling node/es6/ts -> web es5, here I'm doing node es5 -> browser es5 using browserify. With that, code review on a few files, like package.json, notebook/js/main.js, and notebook/js/notebook.js - I think they are a good sample of what's going on. If I get the green light with this, I'll add a require.js bit for backwards compatibility, because as-is require-ing single modules leads to errors, because this PR changes the modules from AMD to commonjs. Adding commonjs support to requirejs isn't bad at all, we can either use write a custom plugin that does it automagically, or use a premade one -> https://github.com/guybedford/cjs . The premade one breaks backwards compatibility, requiring people to use the following syntax in their requirejs paths |
Oh yuck to that |
As long as the dev cycle is fast and easy to set-up I'm ok with that. |
Kyle - can you clarify - I thought you wanted us to move away from AMD On Thu, Aug 13, 2015 at 5:55 PM, Matthias Bussonnier <
Brian E. Granger |
Kyle is speaking of transpiling to AMD. Write as common JS, and make them AMD loadable. |
I'm trying to compromise on what works for the current notebook and widgets which are wedded to requirejs. I'll be checking this PR out again tomorrow. |
As in I'm definitely willing to figure out what needs to be done to make some kind of transition to working in the current notebook. Our new packages should all be commonjs. What I'm saying here is that maybe there's an extra step (as part of notebook js build) to convert to AMD or otherwise adapt. |
Also maybe try the nbgrader extension? On Wed, Aug 26, 2015 at 2:45 PM, Kyle Kelley notifications@github.com
Brian E. Granger |
Please dont' merge such huge things on a rush. |
Just again expressing my frustration that now most of the PR that were ok to merge need to be rebase, and has now we need to discuss wether this was or not 4.1 material or we need to revert, I basically cannot do a thing, as otherwise I'll need to rebase again. I cannot either tell user to rebase theirs as if I tell that, they might have to de-rebase if we revert. 😡 We usually warn 24 to 48h in advance on ML before doing such big change. Nevertheless, despite the rush, and wether we roll back or not I'm happy to see that working. |
I know this work started before we had the conversation about declarative widgets and dashboards which we've implemented as extensions. I suspect this MR breaks what we've done. Two things that would help us as we start to integrate better with the community would be:
|
@parente we would usually not do that this way see this this mail I sent a few hours after: https://groups.google.com/forum/#!topic/jupyter/lkK9rOPEYMc |
Peter, sorry that this dropped with little advanced notice.
On Thu, Aug 27, 2015 at 6:54 AM, Peter Parente notifications@github.com
Brian E. Granger |
Hi, Matthias sorry this went in without getting your input :( I think there are few aspects of this: Communication: This is another example of how our communication model is straining under Merging fast: For the last few weeks, there has been a "merge fast and iterate" effort in Technical details of this PR: There is broad agreement that we are moving torwards having our JS in On Thu, Aug 27, 2015 at 1:09 AM, Matthias Bussonnier <
Brian E. Granger |
When @Carreau and I met this morning, his biggest concern was that we should probably not make this part of 4.x since we're not sure how stable it is and there are bug fixes slated for 4.1. We could create a 4.x branch that predates this PR, and move forward with this as 4.2 or 5.x. |
I'm certainly happy to revert, apply @Carreau's patch that fixes the command palette/search (perhaps we need tests there), and then re-open it for further discussion. |
The other PRs where purely addition and self contained. This is actually a gigantic change that break some (fixable) things, and might also have broken many other things.
I'm fine with this PR, but we need a stable 4.1 have seen how many issues we have with 4.0 ? We can't afford to get a 4.1 which is more work to package than 4.0 and have downstream not upgrading from 4.0 to 4.1 because it breaks things. The rule as always been no refactor or big changes for 4.1, then we fork-of a dev branch and backport. Let's wait for US to finish waking up and respond on ML. We can even wait a few days, but right now, I would be a maintainer of downstream project I would just be pissed of for breaking the meaning of semantic versioning if this is a 4.1 |
Yes, we may want to move this a branch and wait until after 4.1. At the same time, there is an elephant in the room: our JavaScript APIs are Here is my preference: because our JS APIs are not public, we are free to The alternative is to do refactoring in a branch until 5.0, which I don't On Thu, Aug 27, 2015 at 7:33 AM, Matthias Bussonnier <
Brian E. Granger |
Ping @jasongrout |
If we keep this, we can start writing unit tests with mocha directly right now (though we could have with requirejs too). |
I like that ! On Thu, Aug 27, 2015 at 11:55 AM, Kyle Kelley notifications@github.com
Brian E. Granger |
Ok, one other negative that I didn't saw. Moving everything to static-src break the history where all file looks in github like having only 1 commit from Jon. |
Well that's no good. |
I'm currently feeling like we should revert and that we can continue discussions about the PR itself, same as seeing what can be broken out into smaller PRs. |
Is there a way of implementing this stuff without collapsing the git On Fri, Aug 28, 2015 at 4:42 AM, Kyle Kelley notifications@github.com
Brian E. Granger |
The git history is ok, it just confuse most of the tools around because the On Fri, Aug 28, 2015 at 4:21 PM, Brian E. Granger notifications@github.com
|
I guess that is what I am asking. Sometimes we need to move things around On Fri, Aug 28, 2015 at 8:27 AM, Matthias Bussonnier <
Brian E. Granger |
Because the files are being transpiled from one context into another, and they reference each other, sometimes with abs path, they need to live in two separate parent directories. This means the .js files move from static/ to somewhere, or the .less & .css move from /static to somewhere and somewhere becomes the new static directory. I just picked the prior because it made more sense. I tried to move everything with |
Ahh, Ok so the files moving are not API breakages, but they led to files On Fri, Aug 28, 2015 at 5:36 PM, Jonathan Frederic <notifications@github.com
Brian E. Granger |
Gets rid of require.jsLeaves require.js, only for backwards compatibility.Moves a lot of install logic out of setup base
Allows us to use NPM packages directly in our JavaScript! ⚡
No longer minify JS
Pinging interested people @minrk @Carreau @takluyver @rgbkrk .
TODO: